Skip to content

Support a reusable 'var_cache' within the IdentifyVariableVisitor#3966

Open
jsiirola wants to merge 7 commits into
Pyomo:mainfrom
jsiirola:identify_vars_seen_cache
Open

Support a reusable 'var_cache' within the IdentifyVariableVisitor#3966
jsiirola wants to merge 7 commits into
Pyomo:mainfrom
jsiirola:identify_vars_seen_cache

Conversation

@jsiirola
Copy link
Copy Markdown
Member

@jsiirola jsiirola commented May 29, 2026 •
edited
Loading

Fixes # .

Summary/Motivation:

This implements a minor performance improvement when we want to collect all the variables used by a model. The previous implementation ended up using two "seen" caches: one within the IdentifyVariableVisitor, and the other in the get_var_from_components() utility. This allows a client to instantiate an IdentifyVariableVisitor with a persistent "seen" cache, thereby eliminating the redundant caches.

This implementation is fully backwards compatible with the previous IdentifyVariableViaitor / identify_variables implementation.

Changes proposed in this PR:

  • Support a persistent "seen" cache within the IdentifyVariableVisitor
  • Update get_vars_from_components to use a single persistent "seen" cache
  • Add a get_vars() utility that is an alias for get_vars_from_components(ctype=(Constraint, Objective))
  • Test the new behavior

AI-Use Disclosure

  • AI tools were NOT used during the preparation of this PR

or

  • AI tools contributed to the development of this PR

    • AI tools generated documentation (including the PR description/comments, code comments, and/or Sphinx documentation)
    • AI tools generated tests (baselines, examples, and/or code)
    • AI tools generated code (apart from tests)

    Review process (select ONE):

    • Rewritten: All AI-generated content was rewritten by me before being committed.
    • Reviewed/verified: I retained AI-generated content and verified it before committing. Verification included (as applicable):
      • Ran the code and fixed issues
      • Added and ran tests
      • Checked correctness/logic of code and tests
      • Checked for alignment with the contribution guide
      • Considered security implications
    • As-is: AI-generated content was commited directly to the repository

Notes for reviewers (optional):

Legal Acknowledgement

By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the BSD license.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 29, 2026 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.12%. Comparing base (3603b1e) to head (a8f3201).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3966      +/-   ##
==========================================
+ Coverage   90.11%   90.12%   +0.01%     
==========================================
  Files         909      909              
  Lines      108580   108645      +65     
==========================================
+ Hits        97847    97921      +74     
+ Misses      10733    10724       -9
Flag Coverage Δ
builders 29.09% <32.00%> (-0.01%) ⬇️
default 86.11% <100.00%> (?)
expensive 35.13% <96.00%> (?)
linux 87.63% <100.00%> (-1.99%) ⬇️
linux_other 87.63% <100.00%> (+<0.01%) ⬆️
oldsolvers 28.04% <32.00%> (+<0.01%) ⬆️
osx 83.06% <100.00%> (+<0.01%) ⬆️
win 85.38% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jsiirola jsiirola changed the title Support a reusable 'seen' cache within the IdentifyVariableVisitor Support a reusable 'var_cache' within the IdentifyVariableVisitor May 29, 2026
michaelbynum
michaelbynum reviewed May 31, 2026
View reviewed changes
Comment thread pyomo/core/expr/visitor.py
assert not self._expr_stack
self._seen = {}
if self._seen is None:
self._seen = {}
Copy link
Copy Markdown
Contributor

@michaelbynum michaelbynum May 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use a component set?

Copy link
Copy Markdown
Member Author

@jsiirola jsiirola Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly for historical reasons? I think that identify_variables actually predated the modern implementation of ComponentSet, and I was trying to do a minimal change here.

We should consider reworking the implementation to be based on ComponentSet. There might be a slight overhead increase (I am guessing the cost of an extra function call per leaf node) - we should benchmark that. (Although I am not expecting it to show up, as the core things like model generation, compilation, and writers do not use it).

Copy link
Copy Markdown
Member Author

@jsiirola jsiirola Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quick test with PMedian (test8) shows that using a ComponentSet within identify_variables is 10-12% slower. One possibility is that we could advertise that the walker takes a ComponentSet, but internally we operate directly on the underlying _data dictionary?

Copy link
Copy Markdown
Contributor

@michaelbynum michaelbynum Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of using a ComponentSet but operating on the underlying _data dictionary. That is provides a cleaner interface without the performance hit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelbynum michaelbynum michaelbynum left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jsiirola @michaelbynum